⚠️ No Changeset found

Latest commit: 22454da

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

@googlebot I signed it!

@Feiyang1 @hiranya911 @hsubox76
Can get some feedback on this PR/Issue?

@Feiyang1 @hiranya911 @hsubox76
Just pinging you guys again. Would appreciate some feedback here.

The underlying issue has been open for almost three years now. It seems to be an easy fix (the change in this PR is non-breaking btw). If there's something blocking this, it would be great to know about it.

Let me know if there's someone else I should ping to review this.

Thanks, I will discuss this with other teams (functions backend, other platforms such as iOS and Android) to try to get background and make sure we're not missing any context and it won't have any unintended effects. We also probably want changes to be made in sync with the other platforms (iOS, Android).

@hsubox76 Following up on this:
Were you able to bring this issue up with the other Firebase teams? Can we get some feedback on the status of this issue?

I mentioned it before, but the current implementation doesn't work with dates at all. Presumably, anyone using this to send dates is using their own workaround. The change proposed in this PR is non-breaking.

Yes, I did bring it up with the other teams and the larger issue was too complex to reach a conclusion on, but we decided to fix the immediate bug we want to JSON.stringify() the entire data object on the way to the server, so that all data is at least serialized in a predictable, consistent way, instead of the surprise of having a Date be an empty object.

We don't want to automatically deserialize ISO strings returned from the server into Date objects, in case someone intentionally is sending an ISO formatting string back and wants to keep it as a string.

I was planning to make these changes this week but have not had time yet due to a number of pressing beta releases. If you have time to change this PR to reflect the plan above, we can merge it.

A full solution for functions handling special data types on the server side and passing type data back to the deserializer, etc. will probably be a long term feature I'm not sure I can promise any timeline or prioritization on yet.

Thanks for your reply.

I can make the changes to the PR to remove the decoding, but I do think that it would be valuable to have.

I understand the reasoning for not wanting to automatically deserialize ISO strings. I'm obviously not familiar with the implications this would have to the larger issue, but I think it would make sense to encode them to an object like this:

{ _firebaseSerializedDate: `the iso string` }

This way, we can almost guarantee that only Date objects would be serialized/deserialized.

That would definitely be a long-term solution as the server-side function would have to send some kind of standardized wrapper back to indicate the type, and the other platform SDKs (Android, iOS) would have to recognize this wrapper. There was an effort to do this in the past but it was never completed. Would like to see what we can do now but can't make any promises.

We can and should quickly fix the serialization error where a Date ends up as an empty object by JSON.stringifying the entire data object, since that can be entirely handled in the JS SDK. I hope to get to it soon if you don't have time. By the way, if you want to go through with it, this PR will also need a changeset (yarn changeset to add).

Hope you don't mind, I've made a PR. We also needed to make the same change to the "exp" (modularized) version of functions that's currently in beta. #4887

Not at all. I'll close this one then.